Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX Release-1-13] Added deprecation for Ember.SortableMixin. #12471

Merged
merged 1 commit into from
Nov 6, 2015
Merged

[BUGFIX Release-1-13] Added deprecation for Ember.SortableMixin. #12471

merged 1 commit into from
Nov 6, 2015

Conversation

workmanw
Copy link

@workmanw workmanw commented Oct 9, 2015

Addresses #12470. If this is good, I'll add a docs PR to the website.

@workmanw
Copy link
Author

workmanw commented Oct 9, 2015

Hrm, looks like I forgot to run the tests. Oooops. Will fix and squash.

@workmanw
Copy link
Author

workmanw commented Oct 9, 2015

So I updated this to fix the tests ... but I'm looking for some guidance here. What I realized is that Ember.ArrayController included Ember.SortableMixin by default. So I followed Ember.ArrayController's lead and made the deprecation conditional based on if the controller was auto generated. But if a user just defined a plain ole Ember.ArrayController they will get the SortableMixin deprecation warning.

I couldn't find a clear path to only throw the warning if they had explicitly included Ember.SortableMixin or tried to utilize it on an Ember.ArrayController where it was already defined.

Perhaps one solution is to just make the deprecation warning better to notify a user that it is part of the ArrayController and if they're not using it, to ignore it. Thoughts?

@workmanw
Copy link
Author

workmanw commented Nov 3, 2015

Bump. I know there maybe some strings attached to this one, but myself and at least one other person were tripped up by this.

@@ -122,6 +124,11 @@ export default Mixin.create(MutableEnumerable, {
*/
sortFunction: compare,

init() {
Ember.deprecate(sortableMixinDeprecation, this.isGenerated);
this._super.apply(this, arguments);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tweak to this._super(...arguments), and move the super call before the deprecation?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue Sure, will do.

@Gaurav0
Copy link
Contributor

Gaurav0 commented Nov 3, 2015

but I'm looking for some guidance here. What I realized is that Ember.ArrayController included
Ember.SortableMixin by default. So I followed Ember.ArrayController's lead and made the
deprecation conditional based on if the controller was auto generated. But if a user just defined a
plain ole Ember.ArrayController they will get the SortableMixin deprecation warning.

I couldn't find a clear path to only throw the warning if they had explicitly included
Ember.SortableMixin or tried to utilize it on an Ember.ArrayController where it was already defined.

Perhaps one solution is to just make the deprecation warning better to notify a user that it is part of
the ArrayController and if they're not using it, to ignore it. Thoughts?

My thought is if the user explicitly defines an ArrayController, it is probably okay to throw multiple deprecation warnings. Ember 1.13 has so much deprecation spew, what's a little more?

rwjblue added a commit that referenced this pull request Nov 6, 2015
[BUGFIX Release-1-13] Added deprecation for Ember.SortableMixin.
@rwjblue rwjblue merged commit 144caef into emberjs:release-1-13 Nov 6, 2015
@rwjblue
Copy link
Member

rwjblue commented Nov 6, 2015

Thanks @workmanw !

@workmanw
Copy link
Author

workmanw commented Nov 6, 2015

You're welcome @rwjblue! I'll follow this up with a PR to the website.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants